Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use RemoteCredProvider instead of EC2RoleProvider #2983

Merged
merged 4 commits into from
Jul 31, 2017
Merged

Conversation

jefferai
Copy link
Member

@jefferai jefferai commented Jul 7, 2017

This better handles ECS and other future use-cases.

@jefferai jefferai added this to the 0.7.4 milestone Jul 7, 2017
@jefferai
Copy link
Member Author

jefferai commented Jul 7, 2017

@joelthompson It's not letting me request a review from you but can you take a look?

@joelthompson
Copy link
Contributor

@jefferai -- this generally seems fine (modulo the compile error from Travis), but I don't have too much experience with the golang SDK. I would suggest updating the documentation to reflect these changes, i.e.:

  • In the AWS secret backend, for the region option, mention the environment variable, and updating "Note: the client uses the official AWS SDK and will use environment variable or IAM role-provided credentials if available." to also add ECS task role.
  • In the AWS auth backend, where it states, "environment variables, ~/.aws/credentials, IAM instance profile in that order" to add ECS task role
  • The S3 storage backend, for the region option, note that it'll use the AWS_REGION environment variable if AWS_DEFAULT_REGION isn't set (and if you're in there, fix the endpoint parameter doc as it has the wrong environment variable)

There's some more cleanup work that could be done to make things more consistent, e.g., move the DynamoDB physical backend to use the credential helper, as it won't pick up creds from ECS tasks, be consistent about using AWS_REGION and AWS_DEFAULT_REGION environment variables, and make AWS_REGION take precedence over AWS_DEFAULT_REGION which is what the golang SDK does -- though that's a small a breaking change. But, I'd suggest just getting this fix in now for the next release, and potentially do the cleanup work later.

@jefferai
Copy link
Member Author

jefferai commented Jul 7, 2017

be consistent about using AWS_REGION and AWS_DEFAULT_REGION environment variables, and make AWS_REGION take precedence over AWS_DEFAULT_REGION which is what the golang SDK does

I didn't even see AWS_DEFAULT_REGION in the sdk, I thought that was something Vault-specific.

Anyways, I've updated with your feedback, except for updating the docs. Can you take another look?

Copy link
Contributor

@joelthompson joelthompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to factor out the region environment variable checking since it's repeated in a few places.

I didn't even see AWS_DEFAULT_REGION in the sdk, I thought that was something Vault-specific.

See, e.g., this

@@ -31,7 +32,10 @@ func getRootConfig(s logical.Storage) (*aws.Config, error) {
}

if credsConfig.Region == "" {
credsConfig.Region = "us-east-1"
credsConfig.Region = os.Getenv("AWS_REGION")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you also want to check AWS_DEFAULT_REGION here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor

@joelthompson joelthompson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jefferai jefferai modified the milestones: 0.7.4, 0.8.0 Jul 24, 2017
@jefferai jefferai requested review from briankassouf and removed request for chrishoffman July 31, 2017 21:32
@@ -35,10 +35,11 @@ storage "s3" {

- `endpoint` `(string: "")` – Specifies an alternative, AWS compatible, S3
endpoint. This can also be provided via the environment variable
`AWS_DEFAULT_REGION`.
`AWS_S3_ENDPOINT`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this incorrect before?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. Looks like it was copypasta. The code actually uses AWS_S3_ENDPOINT though.

@jefferai jefferai merged commit 54e3d61 into master Jul 31, 2017
@jefferai jefferai deleted the issue-2979 branch July 31, 2017 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants